Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: merge staging to master #38

Merged
merged 39 commits into from
Jul 14, 2022
Merged

ci: merge staging to master #38

merged 39 commits into from
Jul 14, 2022

Conversation

MatrixAI-Bot
Copy link
Member

This is an automatic PR generated by the pipeline CI/CD. This will be automatically fast-forward merged if successful.

* Applied snapshots to dbGet and dbMultiGet, changed dbGetMany to dbMultiGet
* Snapshots work with transactionGet and transactionGetForUpdate
* Testing repeatable read using snapshots
* Turns out snapshot lifecycle is controlled by the transaction, so no manual release
* Transactions now have their own priority work count
* Testing dbClose will automatically close of snapshots, iterators and transactions
* Enabled `NODE_DEBUG_NATIVE` runtime control of rocksdb logging
* Testing transaction iterator
be called when the detached target is already closed. So I've removed
the precondition of `Detach+` methods and `DecrementPendingWork` from
both `Database` and `Transaction` classes.
* Fixed missing promisify for `transactionClear`
* Using `__func__` for log messages
* Namespacing C++ iterator workers
* Testing `transactionClear` with non-repeatable read
Introduce Snapshot Isolation OCC to DBTransaction
@MatrixAI-Bot MatrixAI-Bot self-assigned this Jun 30, 2022
@MatrixAI-Bot
Copy link
Member Author

Pipeline Attempt on 576896307 for 8fd0c9a

https://gitlab.com/MatrixAI/open-source/js-db/-/pipelines/576896307

@ghost
Copy link

ghost commented Jun 30, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Jul 12, 2022

The new 22.05 revision means that we are now using 16.15.0 as the node version.

We need to update all of the pkg-fetch releases. @emmacasolin Can you do an update for js-polykey and ts-demo-lib?

@CMCDragonkai
Copy link
Member

Some bug discoveries:

So basically we need to use npm_config_jobs=max as env variable, and --msvs_version=max as a parameter.

The only thing guaranteed to work is the CLI parameters. The .npmrc is not reliable at all here.

@CMCDragonkai
Copy link
Member

Ok I've gone through the prebuildify code. It does in fact hard code the arguments of: node-gyp rebuild. In fact it forces a --target and --devdir (which is ignored when npm_config_nodedir is set) as well as --arch=x64 and --release.

This means if we want to do incremental compilation, we cannot be using prebuildify.

Since it is quite complicated magic, I'm going to be removing prebuildify, and replacing it with just a simpler output by copying what's in the ./build and pushing it to the ./prebuilds directory based on the structure expected of node-gyp-build.

It seems we just need:

prebuilds/
  darwin-x64+arm64/node.napi.node
  linux-x64/node.napi.node
  win32-x64/node.napi.node

Will need to verify what exactly is the structure, but it appears to be the process.platform, then dash, then process.arch but with x64+arm64 for universal binary, and then finally node.napi.node.

@CMCDragonkai
Copy link
Member

The failure in the windows build might be do with the fact that the name of the native is called rocksdb, while there is a dependency with the same name. That is both binding.gyp and deps/rocksdb/rocksdb.gyp have the name rocksdb.

This isn't a problem on Linux because it uses rocksdb.a to mean the static library from deps/rocksdb, and rocksdb.node for the binding.gyp.

But it appears that on Windows, it is using rocksdb.lib instead of .a, and maybe there's a conflict here...

So I'm going to change the name of binding.gyp target_name to be just db.

@CMCDragonkai
Copy link
Member

I'm attempting some changes on feature-no-prebuildify. By removing the prebuildify, our build will need a special scripts/prebuild.js., that replaces prebuildify. It should do its build incrementally with:

node-gyp configure --msvs_version=2019
node-gyp build --jobs=max",

Compensate for the option bugs that node-gyp has.

Document all the weird behaviour.

And create the relevant prebuilds directory after compilation.

The main reason tsc isn't doing incremental compilation is because it doesn't track removed files. So that's why we use rimraf ./dist. Otherwise the dist may have previously compiled files that is now removed. Track this microsoft/TypeScript#30602.

However as long as the CI/CD tracks the mtimes, while caching ./build, it should be sufficient to proceed.

@CMCDragonkai
Copy link
Member

After renaming to db, it works. No more name conflict, windows compilation succeeds.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Jul 13, 2022

Ok examining the nodeGypBuild.path function I have found the expected structure:

prebuilds/
  <os.platform()>-<os.arch()>(?:\+<os.arch()>)*/
    <tag>(\.<tag>)*\.node

Where the <tag> can be: <runtime>|napi|abi\d+|uv\d+|armv\d+|glibc|musl.

And <runtime> can be node | electron | node-webkit.

Note that it is actually mandatory for there to be an napi|abi\d+. The napi is for ABI backwards/forwards compatibility, while the abi\d+, is for specific process.versions.module.

The tags can be in any order. The \.node is the extension, it must .node.

@CMCDragonkai
Copy link
Member

In our case, it's only relevant to tag the binary if we want to extend our specificity. The default specificity is just to say node and napi which results in node.napi.node.

The arch is interesting because we will need to pass the --arch into the node-gyp which should be x64+arm64.

If the arch is not specified, it defaults to the host platform.

So our ./scripts/prebuild.js can just do something like this:

  1. For platform, always use os.platform(), there's no cross compilation for node-gyp anyway.
  2. For architecture, this can take the default os.arch(), or if specified with --arch, which is also passed to node-gyp configure and node-gyp build.
  3. As for the rest of the tags, we will just fix to node.napi for now. We won't have different runtimes atm between node and electron. The NAPI is compatible between the 2. And none of the other flags are relevant yet.

Future builds which precompile for android or ios may require further tags. In which case the os.platform() may change, and the runtime may also be different, like jsc because react native uses javascript core.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Jul 13, 2022

Note that now that we have a scripts/prebuild.js, the --nodedir has to be set when building within nix. I did this previously in the shell.nix by setting npm_config_nodedir, but this isn't recognised by scripts/prebuild.js.

At the same time in the TS demo lib native, we have --nodedir being set in postInstall. So that would be propagated down to the relevant commands too.

We may need to recognise the npm_config_nodedir option.

Also because the prebuild script runs as prebuild to npm run build, it does not appear to receive options from just npm run build --nodedir=....

Which may mean env variable is the only way to do it.

https://stackoverflow.com/questions/61510927/passing-cli-args-to-pre-command-in-package-json

Which would mean that TS demo lib native has an incorrect line in the utils.nix.

@CMCDragonkai
Copy link
Member

Ok now the script takes into account env variables. We can make use of this now.

@CMCDragonkai
Copy link
Member

Ok there's still a problem with running npm run build on Windows. The reason is this:

The problem is mainly that the PATHEXT is not consulted when execFileSync (and therefore spawnSync) is used. It's only consulted when we use execSync or enable the shell option which causes the command to be executed while in a CMD shell. nodejs/node#10302

This means when we try to execute node-gyp as we normally do in our execFileSync, it ends up with an ENOENT error because it cannot find it.

However if I add the .cmd extension to form node-gyp.cmd, then it does find the file and executes it properly.

@CMCDragonkai
Copy link
Member

Rather than bringing in cross-spawn which I think we actually use in PK (@emmacasolin please confirm), a quick solution is to enable the usage of shell: true option which will just run the program with an intermediate shell.

Alternatively we can add the .cmd extension too when we detect we're on the windows platform, but I think it's easier to just enable the shell rather than hardcoding an extension of the command being executed.

* Added `scripts/prebuild.js` which is set as the `prebuild` script in `package.json`
* The `scripts/prebuild.js` supports `npm_config_devdir`, `npm_config_nodedir` and `npm_config_arch` as environment variables
* Environment variables are necessary to pass configuration when using `npm run build` because the prebuild script doesn't receive commmand line parameters
* Added `engines` which specifies the node runtime and msvs version, used by `scripts/prebuild.js`
* Renamed `src/rocksdb` to `src/native` to avoid name conflict with `deps/rocksdb`, this affects windows builds
* All `package.json` scripts must be prefixed with the actual program to be executable on Windows
* The `execFile` functions do not read `PATHEXT` requiring the usage of `shell: true` option on windows
* Node headers and static libraries are installed into `npm_config_devdir` but only for window and macos, nix still uses `npm_config_nodedir`
* The `scripts/prebuild.js` supports incremental compilation, just need to cache the `./build` directory
* No more `prebuildify`, completely replaced by `scripts/prebuild.js`, replicates expected `./prebuilds` file and directory structure
@CMCDragonkai
Copy link
Member

Ok so the windows build should work now on CI/CD.

Incremental compilation should still be ensured by integrating the caching of the ./build directory in https://docs.gitlab.com/ee/ci/caching/. We may wish to express this globally. This directory is reserved for native building. So no need to do any cache override in the specific build job.

On matrix-win-1, full compilation with all cores took about 7 minutes.

Mac failures still to be looked at. This may have to do with compilation flags.

@MatrixAI-Bot
Copy link
Member Author

Pipeline Attempt on 587061594 for 1f6299d

https://gitlab.com/MatrixAI/open-source/js-db/-/pipelines/587061594

@CMCDragonkai
Copy link
Member

Also important to introduce the chocolatey caching that we have in js-polykey too.

@emmacasolin
Copy link
Contributor

Ok there's still a problem with running npm run build on Windows. The reason is this:

The problem is mainly that the PATHEXT is not consulted when execFileSync (and therefore spawnSync) is used. It's only consulted when we use execSync or enable the shell option which causes the command to be executed while in a CMD shell. nodejs/node#10302

This means when we try to execute node-gyp as we normally do in our execFileSync, it ends up with an ENOENT error because it cannot find it.

However if I add the .cmd extension to form node-gyp.cmd, then it does find the file and executes it properly.

The ENOENT issue had already been brought up here https://github.com/MatrixAI/js-polykey/issues/401#issuecomment-1181364907, but I can add this additional context because I hadn't previously looked into the cause of it

@CMCDragonkai
Copy link
Member

Yea this would only affect commands that are scripts and are not exes. So for PK that might affect our own scripts. But using an intermediate shell is not the solution for PK... I only enabled it here because it's just a script executing more scripts.

@CMCDragonkai
Copy link
Member

Windows CI/CD builds succeeded: https://gitlab.com/MatrixAI/open-source/js-db/-/jobs/2716815439

However mac CI/CD builds still failing: https://gitlab.com/MatrixAI/open-source/js-db/-/jobs/2716815441

@CMCDragonkai
Copy link
Member

Fixed the mac builds, it was a missing comma in the settings. Surprising that the gyp file allows missing commas.

So the mac builds and tests work now on matrix-mac-1.

But I did notice that the compilation settings are a bit messy, worth looking into cleaning it up so it's clearer. Messy settings include multiple overrides of -std and -stdlib, and -mmacosx-version-min1 and -arch. It's also not entirely consistent between the deps and this js-db's native binding.

@MatrixAI-Bot
Copy link
Member Author

Pipeline Attempt on 587539054 for 886f7cb

https://gitlab.com/MatrixAI/open-source/js-db/-/pipelines/587539054

@CMCDragonkai
Copy link
Member

While the mac builds is running on the CI/CD I experimented with how nix could work.

Firstly the latest update to matrix-mac-1 replaced /etc/zshrc and I had to add it back in with sudo vim /etc/zshrc with:

# Nix
if [ -e '/nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh' ]; then
  . '/nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh'
fi
# End Nix

Afterwards, I enabled some settings in /etc/nix/nix.conf... and we can enter nix-shell fine.

Trying to compile results in an error:

error: unknown target CPU 'armv8.3-a+crypto+sha2+aes+crc+fp16+lse+simd+ras+rdm+rcpc'

This is apparently due to the fact that I have x86_64 and arm64 to build a universal binary. However this is not supported yet: https://discourse.nixos.org/t/error-unknown-target-cpu-armv8-3-a/16833.

@CMCDragonkai
Copy link
Member

@emmacasolin can you incorporate chocolatey caching and homebrew caching into the gitlab CI/CD jobs here as well.

@MatrixAI-Bot
Copy link
Member Author

Pipeline Succeeded on 587539054 for 886f7cb

https://gitlab.com/MatrixAI/open-source/js-db/-/pipelines/587539054

@MatrixAI-Bot MatrixAI-Bot merged commit 886f7cb into master Jul 14, 2022
@github-pages github-pages bot temporarily deployed to github-pages July 14, 2022 04:29 Inactive
@emmacasolin
Copy link
Contributor

@emmacasolin can you incorporate chocolatey caching and homebrew caching into the gitlab CI/CD jobs here as well.

Branch created here https://github.com/MatrixAI/js-db/tree/feature-cicd-platforms-caching

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants